[ZEPPELIN-1586] Add security check in NotebookRestApi #1567

anthonycorbacho · 2016-10-29T06:30:17Z

What is this PR for?

Bring some security check in NotebookRestApi.

What type of PR is it?

[Bug Fix | Improvement | Refactoring]

Todos

- Create a proper way to throw webapp error
- Add in NotebookAuthorization some method to check if user is owner, reader or writer
- Add Authorization check in NotebookRestapi
- Add New test for security in notebook rest api

What is the Jira issue?

ZEPPELIN-1586

How should this be tested?

First, force Zeppelin to use auth.

In conf/zeppelin-site.xml change zeppelin.anonymous.allowed to false

<property>
<name>zeppelin.anonymous.allowed</name>
<value>false</value>
<description>Anonymous user allowed by default</description>
</property>

In conf/shiro.ini set Shiro to use Auth at the end of the file

#/** = anon                                                                                                                                           
/** = authc

Start Zeppelin, login and set some permission to a note
try to get a note from Zeppelin Rest Api http://localhost:8080/api/notebook/{noteId} (you can use your browser or curl (if you use curl please add shiro token to curl cookie))

Screenshots (if appropriate)

Questions:

Does the licenses files need update? No
Is there breaking changes for older versions? No
Does this needs documentation? Maybe

minahlee · 2016-11-01T07:54:58Z

zeppelin-server/src/main/java/org/apache/zeppelin/rest/NotebookRestApi.java

+  private void checkIfUserIsOwner(String noteId, String errorMsg) {
+    Set<String> userAndRoles = Sets.newHashSet();
+    userAndRoles.add(SecurityUtils.getPrincipal());
+    userAndRoles.addAll(SecurityUtils.getRoles());


hasReadAuthorization should be changed to isOwner?

Bad copy paste 🗡️

minahlee · 2016-11-01T07:57:22Z

zeppelin-server/src/main/java/org/apache/zeppelin/rest/NotebookRestApi.java

-    }
+
+    checkIfUserCanWrite(noteId,
+        ownerPermissionError(userAndRoles, notebookAuthorization.getOwners(noteId)));


Seems like not only owner but also writer can change permission. Is it intended change? I am asking because this action was only allowed to owner before.

Good catch, i guess a copy pasted so fast :(

minahlee · 2016-11-01T08:25:15Z

zeppelin-server/src/main/java/org/apache/zeppelin/rest/NotebookRestApi.java

-      return new JsonResponse(Status.NOT_FOUND, "note not found.").build();
-    }
+    checkIfNoteIsNotNull(note);
+    checkIfUserIsOwner(noteId,


Since writer has permission to insert paragraph to note, wouldn't it make sense to allow them remove paragraph? For example if someone added new paragraph by mistake, he won't be able to remove it

Yes, it make sense

minahlee · 2016-11-01T08:32:56Z

Thank you for quick response, I went through the code and it looks good to me. It would be nice if you can add some tests. Meanwhile let me build this branch and test it out.

anthonycorbacho · 2016-11-01T08:34:25Z

@minahlee yeah, actually i am doing this right now, I also updated to todo tasks thanks for your review!

minahlee · 2016-11-01T09:28:36Z

I tested some of rest apis and it works well. Next step would be applying same policies to websocket. For example reader cannot change bound interpreter to note via rest api after this PR, but it is possible to do it via websocket(or GUI).

anthonycorbacho · 2016-11-01T09:29:26Z

@minahlee you are right, I guess the next step will be to abstract this logic from rest api and apply to both rest and websocket.

tae-jun · 2016-11-01T14:32:00Z

I also tested and was able to reproduce screenshot above.

When I was another user, it returned 403 status code with message:

{"status":"FORBIDDEN","message":"Insufficient privileges you cannot get this note"}

However, when I didn't log in (i.e. anonymous), the browser(Chrome) redirected me to http://localhost:8080/api/login and returned 405 status code without any message. Users can be confused when there is no error message. And I think 403 status code is more proper since it's forbidden, not method not allowed.

So in my opinion, it would be better:

Send 403 status code with some messages when a user is not logged in. Maybe something like:

{"status":"FORBIDDEN","message":"Please log in"}

This is a miracle feature, by the way 👍

anthonycorbacho · 2016-11-01T14:57:42Z

@tae-jun thanks for the feedback, let me take a look tomorrow

tae-jun · 2016-11-01T15:05:31Z

zeppelin-server/src/test/java/org/apache/zeppelin/rest/AbstractTestRestApi.java

    }
  }
+
+  protected static void startUpWithAutenticationEnabled() throws Exception {


There was a typo! :)

startUpWithAutenticationEnabled -> startUpWithAuthenticationEnabled

anthonycorbacho · 2016-11-02T03:28:03Z

@tae-jun After looking at the code base, I think this case should be handle in another PR, this is kinda out of the scope of this PR and its already becoming super big.

But your made a very valid point here and I guess it deserve a Jira ticket. i will try to find some time to handle this special case.

What do you think?

tae-jun · 2016-11-02T03:46:50Z

@anthonycorbacho Nice! I agree with you :)

I will open the issue on JIRA. But since I don't know much about the code structure, I may need some help 😃

anthonycorbacho · 2016-11-02T03:50:35Z

@tae-jun creating an issue doenst mean that you have to handle it, of course if you want you are welcome to do so, but remember we are a community so that mean we are here to help each others so if you create the PR i can spend some time and work with you on it :)

tae-jun · 2016-11-02T04:39:55Z

@anthonycorbacho Thanks :-) Don't worry! I do this because I want to do 😄

… on anonimous mode or not

…r owner for the given note

…least owner, reader, writer)

…e throw WebAppException

…th shiro (security), I also added some utility http method to do action with authenticated user

minahlee · 2016-11-03T04:20:39Z

Same Spark 1.3.1 test profile failure exists on master which looks like below:

Results :

Tests in error: 
  InterpreterRestApiTest.init:57->AbstractTestRestApi.startUp:233->AbstractTestRestApi.start:201 Â» NullPointer
  CredentialsRestApiTest.init:46->AbstractTestRestApi.startUp:233->AbstractTestRestApi.start:201 Â» NullPointer
  ZeppelinRestApiTest.init:59->AbstractTestRestApi.startUp:233->AbstractTestRestApi.start:201 Â» NullPointer
  NotebookRestApiTest.init:58->AbstractTestRestApi.startUp:233->AbstractTestRestApi.start:201 Â» NullPointer
  SecurityRestApiTest.init:44->AbstractTestRestApi.startUp:233->AbstractTestRestApi.start:201 Â» NullPointer
  NotebookRepoRestApiTest.init:52->AbstractTestRestApi.startUp:233->AbstractTestRestApi.start:201 Â» NullPointer
  ConfigurationsRestApiTest.init:39->AbstractTestRestApi.startUp:233->AbstractTestRestApi.start:201 Â» NullPointer
  ZeppelinSparkClusterTest.init:52->AbstractTestRestApi.startUp:233->AbstractTestRestApi.start:201 Â» NullPointer
  NotebookSecurityRestApiTest.init:49->AbstractTestRestApi.startUpWithAuthenticationEnable:229->AbstractTestRestApi.start:201 Â» NullPointer

Tests run: 9, Failures: 0, Errors: 9, Skipped: 0

So I am going to merge this if there is no more discussion.

tae-jun · 2016-11-03T04:34:36Z

I guess it's same CI error with #1518.

There is a log which is:

gzip: stdin: unexpected end of file
tar: Unexpected EOF in archive
tar: Unexpected EOF in archive
tar: Error is not recoverable: exiting now
+echo 'Unable to extract spark-1.3.1-bin-hadoop2.3.tgz'
Unable to extract spark-1.3.1-bin-hadoop2.3.tgz

This is because of cache failure of Travis CI, and if it happens, it goes forever. Because caching is done only at the first time.

Because of this, if you follow the log more, you can see:

SPARK HOME detected null

Therefore, Zeppelin cannot find Spark, and it goes to failure.

I think it will pass if you run Travis CI on your own repository.

anthonycorbacho · 2016-11-03T04:43:34Z

Yeaaaaaay!

### What is this PR for? Bring some security check in `NotebookRestApi`. ### What type of PR is it? [Bug Fix | Improvement | Refactoring] ### Todos - [x] - Create a proper way to throw webapp error - [x] - Add in `NotebookAuthorization` some method to check if user is owner, reader or writer - [x] - Add Authorization check in `NotebookRestapi` - [x] - Add New test for security in notebook rest api ### What is the Jira issue? - [ZEPPELIN-1586](https://issues.apache.org/jira/browse/ZEPPELIN-1586) ### How should this be tested? First, force Zeppelin to use auth. - In `conf/zeppelin-site.xml` change `zeppelin.anonymous.allowed` to **false** ``` <property> <name>zeppelin.anonymous.allowed</name> <value>false</value> <description>Anonymous user allowed by default</description> </property> ``` - In `conf/shiro.ini` set Shiro to use `Auth` at the end of the file ``` #/** = anon /** = authc ``` - Start Zeppelin, login and set some permission to a note - try to get a note from Zeppelin Rest Api `http://localhost:8080/api/notebook/{noteId}` (you can use your browser or curl (if you use curl please add shiro token to curl cookie)) ### Screenshots (if appropriate) ![note_permission_rest_api](https://cloud.githubusercontent.com/assets/3139557/19827600/ffd68a06-9dea-11e6-8dd5-43f3bd401011.gif) ### Questions: - Does the licenses files need update? No - Is there breaking changes for older versions? No - Does this needs documentation? Maybe Author: Anthony Corbacho <[email protected]> Closes apache#1567 from anthonycorbacho/fix/ZEPPELIN-1586 and squashes the following commits: 6615935 [Anthony Corbacho] Clean anonymous allowed property when shutting down zeppelin server 30815c1 [Anthony Corbacho] Fix typo bab7e60 [Anthony Corbacho] Rewording decd1e9 [Anthony Corbacho] Simple implementation of notebook test with shiro (security) b412266 [Anthony Corbacho] Refactored Abstract rest api test to also handle the case of tests with shiro (security), I also added some utility http method to do action with authenticated user db0c39c [Anthony Corbacho] Adress review and fix typos eacfa8e [Anthony Corbacho] Fix typo and bad copy paste for isOwner c8c42b2 [Anthony Corbacho] Change cxf version from 2.7.7 to 2.7.8 to avoid method not found where throw WebAppException ed404a4 [Anthony Corbacho] Rename permission check note :: be more meaningful 6030776 [Anthony Corbacho] Handle security check fe380ab [Anthony Corbacho] Add webapp exception handler :) 21f9288 [Anthony Corbacho] Replace check of aninonimous by method 0e4cc3c [Anthony Corbacho] Add new method to check if user and roles are member of the note (at least owner, reader, writer) da3415f [Anthony Corbacho] Add new method to help to determinate if user is part of writer and/or owner for the given note 4a43b07 [Anthony Corbacho] Add new method on ZeppelinConfiguration to get is zeppelin is running on anonimous mode or not

…main page ### What is this PR for? - Enables removing note and clear all paragraph's output from Zeppelin main page. - Add rest api for clearing all paragraph output Next possible improvement can be removing notes in folder level and rename folder. ### What type of PR is it? Improvement ### Todos * [x] - Merge #1567 and apply security to `clearAllParagraphOutput` rest api method ### What is the Jira issue? [ZEPPELIN-1564](https://issues.apache.org/jira/browse/ZEPPELIN-1564) ### Screenshots (if appropriate) ![oct-27-2016 18-26-03](https://cloud.githubusercontent.com/assets/8503346/19761938/e013ea02-9c72-11e6-9a08-0a70aca145d2.gif) ### Questions: - Does the licenses files need update? no - Is there breaking changes for older versions? no - Does this needs documentation? yes Author: Mina Lee <[email protected]> Closes #1565 from minahlee/ZEPPELIN-1564 and squashes the following commits: 749aebe [Mina Lee] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1564 1393ee9 [Mina Lee] Rename class name from UnauthorizedException to ForbiddenException Update clear output rest api doc response code 2ee452e [Mina Lee] Add auth check before clearing all paragraph fb7e6ae [Mina Lee] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1564 f349dbf [Mina Lee] Change post to put 7eb3521 [Mina Lee] Give writer permission to clear output dea3ef6 [Mina Lee] Remove unused import d66600c [Mina Lee] Add rest api endpoint for clear paragraph result to document 3d19141 [Mina Lee] Add rest api for clear all paragraph result and add test 98d7604 [Mina Lee] Add clearAllParagraphOutput unit test 4adddb4 [Mina Lee] Clear all paragraphs and remove note from main page

### What is this PR for? After #1567 merged we can get/set note permission info for a certain note. But this info is not described in anywhere for now. So I added "Security" section under [docs/rest-api/rest-notebook.md](https://github.com/apache/zeppelin/blob/master/docs/rest-api/rest-notebook.md). And currently so many operations are placed under one section like below. <img src="https://cloud.githubusercontent.com/assets/10060731/21560964/c55d41cc-cea9-11e6-96ac-68f762c68bff.png" width="400px"> So I split them under each `Note opersions`, `Paragraph operations`, `Cron jobs`, and `Permission` section. <img src="https://cloud.githubusercontent.com/assets/10060731/21799253/ca3bc596-d75b-11e6-82e5-ba440c7f0713.png" width="400px"> ### What type of PR is it? Documentation ### What is the Jira issue? [ZEPPELIN-1877](https://issues.apache.org/jira/browse/ZEPPELIN-1877) ### How should this be tested? Please see the below screenshots :) ### Screenshots (if appropriate) ![screen shot 2017-01-10 at 5 41 51 pm](https://cloud.githubusercontent.com/assets/10060731/21799334/22daa5f0-d75c-11e6-8ecf-6ae9c4c41e2e.png) ![screen shot 2017-01-10 at 5 41 59 pm](https://cloud.githubusercontent.com/assets/10060731/21799335/24da2740-d75c-11e6-802b-b73fbee0db57.png) ### Questions: * Does the licenses files need update? no * Is there breaking changes for older versions? no * Does this needs documentation? no Author: AhyoungRyu <[email protected]> Closes #1825 from AhyoungRyu/ZEPPELIN-1586/docs and squashes the following commits: a8b6506 [AhyoungRyu] Change some operations' ordering based on CRUD 09d22d3 [AhyoungRyu] Add 'Security' section under notebook restapi docs

minahlee mentioned this pull request Oct 31, 2016

[ZEPPELIN-1564] Enable note deletion and paragraph output clear from main page #1565

Closed

1 task

anthonycorbacho closed this Nov 1, 2016

anthonycorbacho reopened this Nov 1, 2016

anthonycorbacho closed this Nov 1, 2016

anthonycorbacho reopened this Nov 1, 2016

anthonycorbacho closed this Nov 1, 2016

anthonycorbacho reopened this Nov 1, 2016

minahlee reviewed Nov 1, 2016

View reviewed changes

tae-jun reviewed Nov 1, 2016

View reviewed changes

anthonycorbacho closed this Nov 2, 2016

anthonycorbacho reopened this Nov 2, 2016

anthonycorbacho added 9 commits November 2, 2016 18:08

Add new method on ZeppelinConfiguration to get is zeppelin is running…

4a43b07

… on anonimous mode or not

Add new method to help to determinate if user is part of writer and/o…

da3415f

…r owner for the given note

Add new method to check if user and roles are member of the note (at …

0e4cc3c

…least owner, reader, writer)

Replace check of aninonimous by method

21f9288

Add webapp exception handler :)

fe380ab

Handle security check

6030776

Rename permission check note :: be more meaningful

ed404a4

Change cxf version from 2.7.7 to 2.7.8 to avoid method not found wher…

c8c42b2

…e throw WebAppException

Fix typo and bad copy paste for isOwner

eacfa8e

anthonycorbacho added 4 commits November 2, 2016 18:08

Adress review and fix typos

db0c39c

Refactored Abstract rest api test to also handle the case of tests wi…

b412266

…th shiro (security), I also added some utility http method to do action with authenticated user

Simple implementation of notebook test with shiro (security)

decd1e9

Rewording

bab7e60

anthonycorbacho closed this Nov 2, 2016

anthonycorbacho reopened this Nov 2, 2016

anthonycorbacho closed this Nov 2, 2016

anthonycorbacho reopened this Nov 2, 2016

anthonycorbacho added 2 commits November 3, 2016 10:50

Fix typo

30815c1

Clean anonymous allowed property when shutting down zeppelin server

6615935

asfgit closed this in 32fe23f Nov 3, 2016

anthonycorbacho deleted the fix/ZEPPELIN-1586 branch November 3, 2016 06:20

AhyoungRyu mentioned this pull request Dec 21, 2016

[ZEPPELIN-1718] Prevent anonymous user to set note permission #1694

Closed

1 task

AhyoungRyu mentioned this pull request Dec 30, 2016

[DOCS][ZEPPELIN-1586] Add 'Security' section under notebook restapi docs #1825

Closed

[ZEPPELIN-1586] Add security check in NotebookRestApi #1567

[ZEPPELIN-1586] Add security check in NotebookRestApi #1567

Uh oh!

Conversation

anthonycorbacho commented Oct 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is this PR for?

What type of PR is it?

Todos

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

Uh oh!

minahlee Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

anthonycorbacho Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

minahlee Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

anthonycorbacho Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

minahlee Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

anthonycorbacho Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

minahlee commented Nov 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anthonycorbacho commented Nov 1, 2016

Uh oh!

minahlee commented Nov 1, 2016

Uh oh!

anthonycorbacho commented Nov 1, 2016

Uh oh!

tae-jun commented Nov 1, 2016

Uh oh!

anthonycorbacho commented Nov 1, 2016

Uh oh!

tae-jun Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

anthonycorbacho commented Nov 2, 2016

Uh oh!

tae-jun commented Nov 2, 2016

Uh oh!

anthonycorbacho commented Nov 2, 2016

Uh oh!

tae-jun commented Nov 2, 2016

Uh oh!

minahlee commented Nov 3, 2016

Uh oh!

tae-jun commented Nov 3, 2016

Uh oh!

anthonycorbacho commented Nov 3, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

anthonycorbacho commented Oct 29, 2016 •

edited

Loading

minahlee commented Nov 1, 2016 •

edited

Loading